[WEB-3410] fix: work item permission and validation #6621
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request refactors issue handling across several components. The changes replace service-based fetching with a hook-based approach in the issue-level component, update parameter extraction logic (switching from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx (1)
117-118:⚠️ Potential issueFix translation key typo.
The translation key
"toast.error "contains an extra space which could cause issues with translation lookups.Apply this fix:
- title: t("toast.error "), + title: t("toast.error"),
🧹 Nitpick comments (3)
web/ce/components/command-palette/modals/issue-level.tsx (2)
16-16: Consider fallback handling for split logic.
You introducedworkItemfor retrieving identifiers. IfworkItemis missing or doesn't follow the<projectIdentifier>-<sequence_id>pattern, the subsequent lines may break. Consider adding a fallback or validation for safety.
39-41: Validate SWR key uniqueness and handle missing values.
Using"ISSUE_DETAIL_${workspaceSlug}_${projectIdentifier}_${sequence_id}"as the key is fine, but handle cases whereprojectIdentifierorsequence_idmight be empty. This prevents potential collisions or null fetches.web/core/components/command-palette/actions/issue-actions/change-priority.tsx (1)
27-27: Validateissueobject presence.
DerivingprojectIdfromissue?.project_idcan fail if theissueis undefined or ifproject_idis missing. Safeguard this logic or provide an appropriate fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/ce/components/command-palette/modals/issue-level.tsx(2 hunks)web/core/components/command-palette/actions/issue-actions/actions-list.tsx(1 hunks)web/core/components/command-palette/actions/issue-actions/change-assignee.tsx(1 hunks)web/core/components/command-palette/actions/issue-actions/change-priority.tsx(1 hunks)web/core/components/command-palette/actions/issue-actions/change-state.tsx(1 hunks)web/core/components/issues/delete-issue-modal.tsx(1 hunks)web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/core/components/issues/delete-issue-modal.tsx
🔇 Additional comments (9)
web/ce/components/command-palette/modals/issue-level.tsx (3)
9-9: Use of new store hook is consistent.
Great job switching touseIssueDetailfor fetching issue details. This aligns with the broader hook-based approach.
23-23: Confirm removal ofissueServicereferences.
This change replaces the service-based approach with thefetchIssueWithIdentifierhook. Make sure all references to the obsoleteissueServiceare removed to avoid inconsistencies.
45-46: Ensure consistent fallback forprojectId.
Defaulting toparamsProjectIdfirst is good. Confirm thatissueDetails?.project_idis never needed ifparamsProjectIdis set, or consider which value to prioritize in edge cases.web/core/components/command-palette/actions/issue-actions/change-priority.tsx (1)
23-23: Confirm minimal router usage.
Using onlyworkspaceSlugfrom the router is a cleaner approach. Ensure that other components dependent onprojectIdfrom the URL are updated to rely on the issue object if that is now the single source of truth.web/core/components/command-palette/actions/issue-actions/change-state.tsx (2)
20-20: Minimal parameter extraction looks good.
Relying onworkspaceSlugalone keeps the router usage streamlined and less error-prone. Confirm that any previous usage ofprojectIdfrom the router is fully deprecated to prevent confusion.
23-23: Check for parallel usage ofgetProjectStates.
You replacedprojectStateswithgetProjectStates(...). Ensure that no stale references exist to the old store variable or that references to it are removed to avoid confusion.web/core/components/command-palette/actions/issue-actions/change-assignee.tsx (1)
21-22: LGTM! Improved parameter handling.The changes simplify parameter extraction and make
projectIdderivation more reliable by sourcing it directly from the issue object.Also applies to: 28-29
web/core/components/command-palette/actions/issue-actions/actions-list.tsx (1)
27-34: LGTM! Enhanced parameter handling with explicit derivation.The changes improve code reliability by deriving
issueIdandprojectIddirectly fromissueDetails, making the source of truth more explicit and consistent.web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx (1)
173-184: LGTM! Enhanced permission validation.The changes improve security by adding granular permission checks at both workspace and project levels.
| const projectIdentifier = workItem?.toString().split("-")[0]; | ||
| const sequence_id = workItem?.toString().split("-")[1]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate workItem split for robust error handling.
Splitting workItem?.toString() by - may cause runtime errors if workItem is undefined or if the string doesn't contain a dash. Add suitable checks or default values.
| const projectId = issue?.project_id; | ||
| const projectStates = getProjectStates(projectId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle null projectId scenario.
Using issue?.project_id is more reliable, but if issue is undefined, getProjectStates(projectId) might return undefined or throw an error. Consider gracefully falling back or showing a more meaningful warning.
sriramveeraghanta
deleted the
fix-work-item-permission-and-validation
branch
* fix: work item permission and validation * fix: command palette * chore: code refactor
Description
This PR includes following changes:
Type of Change
References
[WEB-3410]
Summary by CodeRabbit